-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add list.reverse
intrinsic function
#1758
Conversation
I think this looks good. |
I think the issue is that |
I think the problem in the above comment and #1232 (comment) is exactly same. We might need to create Another approach is that we can add I guess the first approach is better because it helps in resolving #1232. P.S. |
I think adding a |
I added an |
Well you are not using it for wrapping the intrinsic function node in “create_ListReverse”. |
I tried that
But we also need to allow similar to |
Yes. |
I would use IntrinsicFunction.
The Ok, so the current design already allows to "return nil". I don't think any change is needed? Maybe in ASR passes. |
There is one needed. The following one, diff --git a/src/libasr/ASR.asdl b/src/libasr/ASR.asdl
index 12ada08a1..bdaa0ea1d 100644
--- a/src/libasr/ASR.asdl
+++ b/src/libasr/ASR.asdl
@@ -225,7 +225,7 @@ expr
| FunctionCall(symbol name, symbol? original_name, call_arg* args,
ttype type, expr? value, expr? dt)
| IntrinsicFunction(int intrinsic_id, expr* args, int overload_id,
- ttype type, expr? value)
+ ttype? type, expr? value)
| StructTypeConstructor(symbol dt_sym, call_arg* args, ttype type, expr? value)
| EnumTypeConstructor(symbol dt_sym, expr* args, ttype type, expr? value)
| UnionTypeConstructor(symbol dt_sym, expr* args, ttype type, expr? value) We do the same with Line 365 in bf59fb4
I think after addition of |
I don't think that's the right change. The Oh, I just noticed that I can see a couple issues here:
So your change is probably ok after all. I was confused. I think I keep confusing that Here is one issue: what if we assign an intrinsic function to a function pointer? I don't know if Fortran allows it. Our current design does not allow it. We would have to "wrap" the intrinsic in a regular function. It's probably fine, since these intrinsic functions are often "generic", instantiated to the specific types. So we treat them as part of the language, not really "functions", but "intrinsic operations". The difference is that "intrinsic operations" are generic, instantiated, they do not have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good but please resolve the conflicts (update main
locally and then merge into the branch of this PR, git will ask you to resolve the conflicts, do it, add and commit the changes). Do a clean build afterwards, see if all the tests pass and then push.
src/libasr/codegen/asr_to_llvm.cpp
Outdated
@@ -1887,6 +1887,11 @@ class ASRToLLVMVisitor : public ASR::BaseVisitor<ASRToLLVMVisitor> | |||
dict_type->m_value_type, name2memidx); | |||
} | |||
|
|||
void visit_Expr(const ASR::Expr_t& x) { | |||
// std::cout << "inside visit Expr" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line.
merge `main` into `list_reverse`
Seems like some syntax creeped in while resolving conflicts. @kabra1110 Do you need any help in fixing or would you able to deal with it on your own. |
@certik Does this look good to you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine.
Currently gives an
Internal Compiler Error
atwith test